[SPARK-35270][SQL][CORE] Remove the use of guava in order to upgrade guava version to 27 #32395
Conversation
|
Can one of the admins verify this patch? |
srowen
left a comment
srowen
left a comment
There was a problem hiding this comment.
Looks good, avoiding Guava in favor of the JDK classes.
Is that all the usage of com.google.common.base.Objects ?
There was a problem hiding this comment.
It isn't super important here I think, but does this result in the same string?
There was a problem hiding this comment.
The results are different. Using Objects to return is like "AppShuffleId{appId=appId, shuffleId=100}", using ToStringBuilder to return is like "RemoteBlockPushResolver.AppShuffleId[appId=appId,shuffleId=100]". Will it cause some problems?
There was a problem hiding this comment.
Like with hash function changes, it shouldn't matter to programs. But if some program did rely on it, directly or accidentally, this might break. It's a tough call - how much is the change worth? overall it's an OK improvement but yeah I'm hesitant for just this reason. It's more the hash change than this one.
|
@wForget Spark master branch has already moved to use shaded Hadoop client by default (see SPARK-33212) which effectively isolated itself from the Guava version on the Hadoop side. Did you actually see a Guava conflict issue? |
|
+1 for @sunchao 's comment. Also, Apache Spark 3.2 is moving toward to Hadoop 3.3.x. |
|
It may be unnecessary for the reason above; it still probably wouldn't hurt to just move these to standard JDK classes. I have a little bit of worry about changing behavior, with a possibly different hash or toString, though |
|
@srowen yes agreed - it's better to avoid Guava usage in general if it's not necessary. |
|
Ya, BTW, it seems that we need to revise the wrong title and PR description about |
|
@sunchao @dongjoon-hyun @srowen |
wForget
changed the title
|
@wForget can you enable GitHub Actions in your forked repository? https://github.com/apache/spark/pull/32395/checks?check_run_id=2465058510 |
wForget
force-pushed
the
master-gauva-compatible
branch
from
28df0ec to
46e1eee
Compare
|
@HyukjinKwon I have enabled it, how to rerun these checks? |
|
Did you do something like #32400 (comment) too? If it's done, feel free to rebase which should retrigger the test. |
…guava version to 27.
wForget
force-pushed
the
master-gauva-compatible
branch
from
46e1eee to
d37c843
Compare
|
Seems |
|
I think the concern about changing behavior still stands? |
|
any update? |
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
7 participants
What changes were proposed in this pull request?
Remove the use of guava in order to upgrade guava version to 27.
Why are the changes needed?
Hadoop 3.2.2 uses Guava 27, the change is for the guava version upgrade.
Does this PR introduce any user-facing change?
no
How was this patch tested?
Modify the guava version to 27.0-jre, and then compile.